In the spec PR I don't think we say this method is const, so that is something to update in the spec

If some of the jenkins jobs are still using gcc 7.5, then those [[maybe_unused]] in the constructor will cause a failure because of a gcc bug, see #17736

But I guess it will show up on the pre-commit CI too. Maybe the gcc version was bumped already.

I changed both dynamic_work_group_memory and dynamic_local_accessor to use a solution similar to the one in #17736

Thanks for pointing this out!

Why is this required for dynamic work group memory but not the existing work group mem set_arg() which looks to be doing basically the same thing?

Because dynamic_work_group_memory_base does not exist for device code. I also think it is good practice to make clear that this code does nothing for device compilation.

I also think it is good practice to make clear that this code does nothing for device compilation.

I don't know that I agree with this. Much of the runtime implementation is not relevant for device compilation but guarding everything would be impractical.

Is there a reason we can't make the base class available on device and just guard the impl that actually does the host work?

I think that guarding the impl would increase the amount of #ifdefs that need to be added to extend the base class to the device (e.g. different versions of constructors when creating the impl and when not). Doing it this way, we can keep the #ifdefs restricted to the dynamic_work_group_memory / dynamic_local_accessor class and the base classes will be cleaner. So it's mostly a matter of choosing where to have the noisy #ifdefs I think.

In the end, I think it's a matter of preference. But more importantly, I also chose this way because it's the way that local_accessor / accessor are implemented. For example:

  // setArgHelper for local accessor argument (legacy accessor interface)
  template <typename DataT, int Dims, access::mode AccessMode,
            access::placeholder IsPlaceholder>
  void setArgHelper(int ArgIndex,
                    accessor<DataT, Dims, AccessMode, access::target::local,
                             IsPlaceholder> &&Arg) {
    (void)ArgIndex;
    (void)Arg;
#ifndef __SYCL_DEVICE_ONLY__
    setLocalAccessorArgHelper(ArgIndex, Arg);
#endif
  }

  // setArgHelper for local accessor argument (legacy accessor interface)
  template <typename DataT, int Dims, access::mode AccessMode,
            access::placeholder IsPlaceholder>
  void setArgHelper(int ArgIndex,
                    accessor<DataT, Dims, AccessMode, access::target::local,
                             IsPlaceholder> &&Arg) {
    (void)ArgIndex;
    (void)Arg;
#ifndef __SYCL_DEVICE_ONLY__
    setLocalAccessorArgHelper(ArgIndex, Arg);
#endif
  }

I could also move the #ifdef to setArgHelper to have the same pattern as accessor / local_accessor?

This comment doesn't make sense now since it is checking that it doesn't throw. The error for this was removed in #18199 .

I think I messed up the rebase. The code on origin still has the comment but slightly changed. I will update to match what is in there.